Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Unitary-Hack] Lint rule: Use functions #1579

Merged
merged 37 commits into from
Jun 10, 2024

Conversation

Manvi-Agrawal
Copy link
Contributor

@Manvi-Agrawal Manvi-Agrawal commented May 29, 2024

Fixes #1471.

Future TODO

image
image

Based on https://github.com/microsoft/qsharp/blob/main/compiler/qsc_passes/src/callable_limits.rs

Note to reviewers

Main changes are in qsc_linter folder. Changes in other tests are due to this lint rule complaining about NeedlessOperation, so I converted to function and it changes span accordingly.

@Manvi-Agrawal Manvi-Agrawal changed the title [Unitary-Hack] Use functions [Unitary-Hack] Lint rule: Use functions May 29, 2024
@minestarks
Copy link
Member

Thanks @Manvi-Agrawal ! Would you mind resolving the conflicts in the branch so I can kick off the checks for this PR? I'll give your questions some more thought once I get the feedback from the unit tests in the CI.

@Manvi-Agrawal
Copy link
Contributor Author

Manvi-Agrawal commented May 30, 2024

Thanks @Manvi-Agrawal ! Would you mind resolving the conflicts in the branch so I can kick off the checks for this PR? I'll give your questions some more thought once I get the feedback from the unit tests in the CI.

Done @minestarks. I was on it, and when I came back I saw your comment :-). Also, could we please connect over discord, I have few questions about other open issues xD

Copy link
Contributor

@orpuente-MS orpuente-MS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few suggestions to convert some more comments to docstrings.

@Manvi-Agrawal
Copy link
Contributor Author

Could you please run your local playground with the browser Developer Tools enabled, and watch the JavaScript console logs? It's reporting some panics from the Rust layer when you trigger the lint. Please ensure no panics / error messages show up in regular testing in the playground.

Nice catch @minestarks. I realized now that these were originating from unimplemented quick-fix. I had it as TODO, and converting it to no-op(similar to DivisionByZero) made rust happy :-)

@Manvi-Agrawal Manvi-Agrawal requested a review from orpuente-MS June 4, 2024 18:36
@Manvi-Agrawal
Copy link
Contributor Author

Hi @minestarks , @orpuente-MS , I have addressed all review comments. Could you please take a look?

Copy link
Contributor

@orpuente-MS orpuente-MS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Manvi-Agrawal looks good! I will approve.

@Manvi-Agrawal
Copy link
Contributor Author

@Manvi-Agrawal looks good! I will approve.

Thanks a ton @orpuente-MS for your review and approval.

@Manvi-Agrawal
Copy link
Contributor Author

Hi @minestarks , do you have any other feedback for this PR. Rust panic was really good edge case. Your todays feedback would give me more bandwidth since I can work over that over the weekend, because hack ends on June 12

Copy link
Member

@minestarks minestarks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, thanks. Minor cosmetic improvements and then we should be good to go.

@minestarks minestarks enabled auto-merge June 8, 2024 15:57
@minestarks minestarks disabled auto-merge June 8, 2024 15:58
@minestarks
Copy link
Member

Just needs one more signoff from @DmitryVasilevsky, @cesarzc, @sezna, @swernli, @tcNickolas (probably due to the small sample update) .

@minestarks minestarks added this pull request to the merge queue Jun 10, 2024
Merged via the queue into microsoft:main with commit 2454f09 Jun 10, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lint rule: operations that don't manipulate quantum state can be functions instead
4 participants